Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unique tokenEntity id by both image and media #225

Merged
merged 3 commits into from
Feb 24, 2024

Conversation

daiagi
Copy link
Contributor

@daiagi daiagi commented Feb 23, 2024

PR Type

  • Bugfix

Context

change from tokenId = <collection_id>-<md5(image ?? media)>
to tokenId = <collectionId>-md5(imge + media)

Did your issue had any of the "$" label on it?

@daiagi daiagi requested a review from vikiival February 23, 2024 09:46
@daiagi
Copy link
Contributor Author

daiagi commented Feb 23, 2024

tested on local machine on AHK
works

Comment on lines 8 to 15
export function generateTokenId(collectionId: string, nft: NE): string | undefined {
if (!nft.image && !nft.media) {
warn(OPERATION, `MISSING NFT MEDIA ${nft.id}`)
return undefined
}

return nftMedia
const image = nft.image ?? ''
const media = nft.media ?? ''
const mediaHash = md5(image + media)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check #224 pls

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what the difference?

md5(meta.image) + ':' md5(meta.animation_url) is opaque just same as md5(image + media) is opaque

also both image and animation_utl can be null | undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plus it should be unique per collection
could be a situation where 2 different collection each have same artwork

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the sake of speeding things along i complied
but i do not understand what difference does it make

@vikiival vikiival merged commit 373a560 into kodadot:main Feb 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TokenEntity is merging nfts that has placeholder as an image
2 participants